fix: stop tearing down healthy HTTP sessions on transient probe misses (#1207)#1237
Open
Scriptwonder wants to merge 2 commits into
Open
fix: stop tearing down healthy HTTP sessions on transient probe misses (#1207)#1237Scriptwonder wants to merge 2 commits into
Scriptwonder wants to merge 2 commits into
Conversation
CoplayDev#1207) The orphaned-session detector ended an active session on a SINGLE stale reachability reading, and the reading came from a lone 50ms TCP connect cached for 0.75s — trivially false-negative on a machine busy with test runs or domain reloads. Evidence bundles in CoplayDev#1207 show 13 teardowns and 147 socket closures in one session from exactly this loop, wedging the bridge in no_unity_session churn until manual recovery. - Require 3 consecutive failed polls (0.75s cadence) before declaring a session orphaned; probe readings taken while the editor is compiling or importing don't count toward teardown, and detection is skipped entirely while busy. - Raise the probe's connect wait 50ms -> 250ms, as an overall budget shared across candidate hosts so the worst-case main-thread wait cannot multiply. - Honor UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S above 20s (ceiling now 120s; default unchanged): the old ceiling equalled the default, silently neutering the documented escape hatch for projects whose reloads or test boundaries legitimately exceed 20s. Same treatment for UNITY_MCP_SESSION_READY_WAIT_SECONDS, and both now share one bounded env-read helper. The remaining piece of CoplayDev#1207 (keepalive reload-awareness in WebSocketTransportClient) is untouched here: the resume machinery reworked in CoplayDev#1234 already covers reload boundaries, and the detector debounce removes the dominant churn source dsarno identified.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUnity editor local HTTP reachability probing now uses a longer shared timeout budget, and orphaned session teardown waits for repeated down probes. Server-side wait-time environment parsing is centralized with a higher clamp ceiling and new tests. ChangesUnity Editor: Reachability probing and orphan-session teardown
Estimated code review effort: 3 (Moderate) | ~30 minutes Server-side bounded wait-time environment handling
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant UpdateStartHttpButtonState
participant UpdateConnectionStatus
participant ShouldEndOrphanedSession
UpdateStartHttpButtonState->>UpdateStartHttpButtonState: probe local HTTP server reachability
UpdateStartHttpButtonState->>UpdateStartHttpButtonState: increment/reset consecutiveServerDownPolls
UpdateConnectionStatus->>ShouldEndOrphanedSession: check editorBusy, toggleInProgress, downPollCount
ShouldEndOrphanedSession-->>UpdateConnectionStatus: return true/false
UpdateConnectionStatus->>UpdateConnectionStatus: end session if true
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
342-366: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset the orphaned-poll counter when the session starts
consecutiveServerDownPollsonly clears afterUpdateStartHttpButtonState()sees the server up again, so a stale count can survive a successfulStartAsync()and immediately satisfy orphan detection before the next fresh probe. Clear it when the session transitions to running.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs` around lines 342 - 366, The orphaned-session detection in UpdateConnectionStatus can be triggered by a stale consecutiveServerDownPolls count after the session transitions to running. Reset consecutiveServerDownPolls when the connection successfully starts, and make sure the reset happens in the start-path tied to StartAsync/UpdateStartHttpButtonState so ShouldEndOrphanedSession only uses fresh probes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs`:
- Around line 342-366: The orphaned-session detection in UpdateConnectionStatus
can be triggered by a stale consecutiveServerDownPolls count after the session
transitions to running. Reset consecutiveServerDownPolls when the connection
successfully starts, and make sure the reset happens in the start-path tied to
StartAsync/UpdateStartHttpButtonState so ShouldEndOrphanedSession only uses
fresh probes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39ada2ce-19ef-4b3b-955c-1851ab24acdd
📒 Files selected for processing (6)
MCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csServer/src/transport/plugin_hub.pyServer/tests/test_plugin_hub_wait_env.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.cs.meta
Review follow-up: a streak accumulated while no session was running (detector inert, counter still counting) survived into a freshly started session and could satisfy orphan detection before the first post-start probe refreshed. Reset on the not-running -> running transition, which covers every start path (manual Connect, auto-start, resume) since UpdateConnectionStatus runs on the UI tick.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces unnecessary HTTP transport session churn by debouncing the Unity-side “orphaned session” teardown logic, making local reachability probing less hair-trigger, and by allowing the server-side session resolve/ready wait env overrides to extend beyond the previous hard 20s ceiling.
Changes:
- Debounce orphaned-session teardown to require 3 consecutive failed reachability polls, and skip/defer teardown while the editor is busy (compile/import) or a toggle is in progress.
- Increase local HTTP reachability probe connect budget (50ms → 250ms) and share that budget across candidate hosts to cap worst-case UI-thread blocking.
- Add a bounded env-read helper for PluginHub waits and extend max clamp to 120s, with new unit tests.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.cs.meta | Adds Unity asset metadata for the new EditMode regression test. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.cs | Adds EditMode regression tests for the orphaned-session teardown predicate. |
| Server/tests/test_plugin_hub_wait_env.py | Adds tests for bounded/clamped env overrides for session resolve waits. |
| Server/src/transport/plugin_hub.py | Introduces shared bounded env parsing for resolve/ready waits and raises the clamp ceiling. |
| MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs | Implements debounced orphan detection with busy-editor deferral and a testable predicate. |
| MCPForUnity/Editor/Services/ServerManagementService.cs | Increases local probe timeout and applies a shared overall timeout budget across probe hosts. |
Files not reviewed (1)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/McpConnectionSectionOrphanDetectionTests.cs.meta: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+49
to
+54
| try: | ||
| value = float(raw) | ||
| except ValueError as e: | ||
| logger.warning("Invalid %s=%r, using default %s: %s", name, raw, default_s, e) | ||
| value = default_s | ||
| return max(0.0, min(value, max_s)) |
Comment on lines
+33
to
+35
| def test_invalid_falls_back_to_default(monkeypatch): | ||
| monkeypatch.setenv(ENV, "not-a-number") | ||
| assert _read_bounded_wait_env(ENV, default_s=20.0, max_s=120.0) == 20.0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the dominant churn source in #1207, following the fix order @dsarno proposed from the evidence bundle.
Problem
Two amplifiers turned ordinary reload/test boundaries into
no_unity_sessionwedges:McpConnectionSection.UpdateConnectionStatusends an active session the moment one cached reachability reading is false — and that reading is a lone TCP connect with a 50 ms wait, cached 0.75 s. On a machine busy with a test run or domain reload the probe times out against a perfectly healthy server; the detector then force-stops the bridge (13 teardowns / 147 socket closures in one session in the macOS / HTTP transport: bridge WebSocket repeatedly drops (close 1005) on reload/test boundaries → no_unity_session #1207 evidence bundle), feeding the reconnect churn that outruns the server's session-resolve window.UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_Sis documented as configurable but was clamped to a ceiling equal to its default (20 s), so users whose reloads legitimately exceed 20 s could not actually extend the window.Fix
internal staticpredicate with tests.UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_Sceiling raised to 120 s (default unchanged at 20 s);UNITY_MCP_SESSION_READY_WAIT_SECONDSlikewise (default 6 s). Both now share one bounded env-read helper with tests.What this deliberately does not do
WebSocketTransportClient— fix(editor): survive domain reloads in HTTP auto-start and reload-resume (#1229) #1234's resume rework already owns reload boundaries, and per the maintainer analysis the detector debounce removes the dominant churn source. If churn persists after both land, that's the next lever.Verification
cd Server && uv run pytest tests/ -q→ 1313 passed, 3 skipped (5 new env-clamp tests).tools/check-unity-versions.sh(2021.3 floor passes locally).Summary by CodeRabbit